-
-
Notifications
You must be signed in to change notification settings - Fork 644
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feature: add ability to copy comment link #981
Conversation
@indyteo Would you like to rebase this branch on Then we could try to find someone for a review. Eventually you can also request a review? |
Hi @almereyda, I can have a look if it's not much work 👍 |
@indyteo Hi! Few years later.... is there much demand for this, I'm happy to review it if you want to rebase / merge from main, although I appreciate it's probably not the highest thing on your priority list right now :) |
Hi @mattwoberts, Let's be honest, I completely forgot about this one! 😄 I'd be happy to rebase (and btw clean that awful git history with all those merge commits...) so you can review. After a (very) quick review of the work that was done, I guess it shouldn't cause much trouble to rebase. Thanks |
a95b7d6
to
b27f5a0
Compare
@mattwoberts yay! 🎉 This should be up-to-date, ready to review! 😊 |
Great, thanks :)
I'll take a look soon!
…On Tue, Sep 10, 2024 at 5:12 PM Théo SZANTO ***@***.***> wrote:
@mattwoberts <https://github.com/mattwoberts> yay! 🎉
This should be up-to-date, ready to review! 😊
The git history is definitely cleaner too 😄
—
Reply to this email directly, view it on GitHub
<#981 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA2VDHUN7SE53LF5GHE5IDZV4K57AVCNFSM6AAAAABN4PRXNWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBRGM4DMOJZGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
b27f5a0
to
4877d1c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome stuff - some minor copy edit suggestions but all looks good!
4877d1c
to
d7eca42
Compare
1eda12e
to
20a789e
Compare
In it comes :) |
This is so nice and useful! Looking forward to the next release. |
@almereyda it just got released to v0.23.0 😄🎉 |
@indyteo You beat me to it ;) |
Feature request: Ability to copy comments permalink
As mentioned in the post, it is inspired by GitHub's "Copy Link" feature on comments.
It first add a unique link for each comment, based on their ID.
For example, my reply on this post has the ID
#31419
, so the link would look like :Then, it add the corresponding HTML
id
attribute on a wrappingdiv
around comments, so the page automatically scroll to the targeted comment.On the other hand, it also create a
highlighted
state in theShowComment
component that slightly mutate the comment background (a bit darker than others) to stand out. When a click is performed outside of the comment's area, it get back to normal and the hash is removed from the URL (the part after the#
).Finally, an action in the comment's drop down, target the current comment (set the URL in the navigation bar, triggering the comment highlight and scroll), and copy that link into the user's clipboard. A feedback indicate whether the copy was successful or not (due to the lack of support of the Clipboard API by some old outdated browsers), and invite people to manually copy their page's URL if not.
If the hash don't match anything is simply get ignored, and if it looks like a comment reference but we're unable to find it, a notice is shown to the user, because it may happen when the original comment got deleted, or the URL was manually modified and is now invalid. It get instantly cleaned up to avoid further share of a partially invalid link.
I never really played with URL's hash before and found it funny to do this, this is why I decided to train with this idea, even if it only has 2 votes (which means none, except from the submitter and me 😆), and it only took me about an hour so it was fun 😄